Skip to content

Conversation

@hzeller
Copy link
Member

@hzeller hzeller commented Sep 26, 2025

Also looks like c_api.cc does not depend on c_api_vast.h anymore.

@hzeller hzeller requested a review from cdleary September 26, 2025 08:41
@hzeller
Copy link
Member Author

hzeller commented Oct 2, 2025

Before sending this to XLS internal reviewers, does this look good @cdleary ?

@hzeller hzeller requested a review from meheff October 11, 2025 11:41
@hzeller hzeller force-pushed the feature-20250926-fix-dep-public branch from 01b0f1b to ab6408d Compare October 11, 2025 11:44
@grebe
Copy link
Contributor

grebe commented Oct 13, 2025

Do you still want to land? Is this superseded by #3184?

@hzeller
Copy link
Member Author

hzeller commented Oct 14, 2025

Yes, still want to land.
In #3184 I explicitly broke out the changes in xls/public as I was waiting on this one separately.

@grebe
Copy link
Contributor

grebe commented Oct 16, 2025

Well, this looks good to me. I'm inclined to land unless @cdleary opposes, it's also not hard to revert/fix later if need be.

@cdleary
Copy link
Collaborator

cdleary commented Oct 16, 2025

I think for the header it's useful to have the central .h file roll up all the public APIs, for the cc doesn't matter though.

@grebe
Copy link
Contributor

grebe commented Oct 16, 2025

At least internally, it looks like xlsynth relies on the BUILD dep else a linker error complaining about missing symbols from c_api_vast. @cdleary do you want to keep the BUILD dep or explicitly depend on c_api_vast in xlsynth?

@cdleary
Copy link
Collaborator

cdleary commented Oct 16, 2025

@grebe I need a target that rolls up all the C API symbols as a DSO, that was libxls.so which is the shared library wrapper of c_api AFAIK -- it'd be nice to not break that for header cleanliness :-) we need something that is canonically "libxls with all its symbols"

@grebe
Copy link
Contributor

grebe commented Oct 16, 2025

Makes sense, I think an IWYU: pragma export makes sense then as that will keep the bazel dep as well. @hzeller do you mind making the change?

Also looks like c_api.cc does not depend on c_api_vast.h
anymore.
@hzeller hzeller force-pushed the feature-20250926-fix-dep-public branch from ab6408d to 3c09006 Compare October 20, 2025 07:33
@hzeller
Copy link
Member Author

hzeller commented Oct 20, 2025

Done. Added the // IWYU: pragma export and re-added the :c_api_vast to //xls/public:c_api

@hzeller hzeller force-pushed the feature-20250926-fix-dep-public branch from 3c09006 to 01c4896 Compare October 20, 2025 07:59
@hzeller
Copy link
Member Author

hzeller commented Oct 20, 2025

the ci failure looks unrelated.

@grebe
Copy link
Contributor

grebe commented Oct 21, 2025

Yeah, CI is a known issue.

@grebe grebe self-requested a review October 21, 2025 16:56
@copybara-service copybara-service bot merged commit 2af99f8 into google:main Oct 23, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants